Fix: Use Patch instead of Update to add/remove finalizers #1801
Conversation
Baarsgaard
requested review from
NissesSenap,
weisdd,
ishanjainn,
theSuess,
hubeadmin and
pb82
as code owners
Baarsgaard
force-pushed
the
fix_finalizer_cfg_drift
branch
from
172cf54 to
1fa382c
Compare
Baarsgaard
changed the title
controllers/controller_shared.go
Outdated
| // Remove finalizer through a MergePatch | ||
| // Avoids updating the entire object and only changes the finalizers | ||
| func removeFinalizer(ctx context.Context, cl client.Client, cr client.Object) error { | ||
| patch := []byte(`{"metadata":{"finalizers":null}}`) |
There was a problem hiding this comment.
Although it's nice that you can fix the drift with these changes, I would disagree with the way how patches are formed. - Kubernetes objects may have multiple finalizers, whereas, in this PR, there is an assumption that there will always be a single finalizer (owned by the grafana-operator). - Some users might have their own finalizers (think of custom operators).
Since controllerutil has two functions that safely add (AddFinalizer) / remove (RemoveFinalizer ) a particular finalizer, you can, for instance, have them modify finalizers in a CR object and then prepare a patch by copying value from metadata.finalizers.
There was a problem hiding this comment.
Sure, I'll try and whip up a solution that safely patches the finalizers.
There was a problem hiding this comment.
I ended up almost copying the controllerUtil.RemoveFinalizer implementation for removing a finalizer as it worked quite well already.
There was a problem hiding this comment.
I'm not sure if we should copy code from anywhere. Do you see any benefits compared to direct calls to their library? - Like it was mentioned in my previous comment, I thought of rather calling controllerutil.AddFinalizer / controllerutil.RemoveFinalizer and then copying finalizers field from the result. - It's pretty straightforward, we don't have to maintain code of the underlying functions, and it doesn't create licensing complexity (I think you have to mention authors when copying Apache2 code).
There was a problem hiding this comment.
I partially misunderstood your previous message.
This is iteration aligns with your suggestions!
Baarsgaard
force-pushed
the
fix_finalizer_cfg_drift
branch
2 times, most recently
from
cdde804 to
b778a52
Compare
|
Once this is merged, I will likely add finalizers to |
Baarsgaard
force-pushed
the
fix_finalizer_cfg_drift
branch
from
b778a52 to
a99dc26
Compare
Managed to solve the perpetual drift noted in #1776 that is rooted in us applying finalizers and updating unintended fields on CRs.
I first attempted to use
JSONPatch, but I could not for the life of me get the libraries to comply, despite handcrafting a working patches with kubectl.MergePatchworked immediately.Currently, the implementation is hardcoded to only support arrays of one finalizer. More work can be done to make this support multiple if the need ever arises.